Skip to content

refactor: migrate metal_vlan to equinix-sdk-go#782

Closed
ctreatma wants to merge 7 commits intomainfrom
metal-vlan-sdk
Closed

refactor: migrate metal_vlan to equinix-sdk-go#782
ctreatma wants to merge 7 commits intomainfrom
metal-vlan-sdk

Conversation

@ctreatma
Copy link
Contributor

No description provided.

@displague
Copy link
Member

displague commented Sep 25, 2024

I ran across the some of the same linter violations (and addressed them) in 7fb2dce

Feel free to cherry-pick or solve this problem in another way so that I can keep #622 simpler :-)

@ctreatma
Copy link
Contributor Author

I left the device linter errors in place here because they are already fixed in #707 which is explicitly focused on the device code.

@displague
Copy link
Member

#707 has been merged - rebase away!

name: "ComplexMatch",
args: args{
vlans: []packngo.VirtualNetwork{{VXLAN: 987, FacilityCode: "fac", MetroCode: "skip"}, {VXLAN: 123, FacilityCode: "fac", MetroCode: "met"}, {VXLAN: 456, FacilityCode: "fac", MetroCode: "nope"}},
vlans: []metalv1.VirtualNetwork{{Vxlan: metalv1.PtrInt32(987), AdditionalProperties: map[string]interface{}{"facility_code": "fac"}, MetroCode: metalv1.PtrString("skip")}, {Vxlan: metalv1.PtrInt32(123), AdditionalProperties: map[string]interface{}{"facility_code": "fac"}, MetroCode: metalv1.PtrString("met")}, {Vxlan: metalv1.PtrInt32(456), AdditionalProperties: map[string]interface{}{"facility_code": "fac"}, MetroCode: metalv1.PtrString("nope")}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: breaking this up with newlines may help readers without line-wrapping views

displague
displague previously approved these changes Sep 25, 2024
if vlan.Description != "" {
m.Description = types.StringValue(vlan.Description)
if vlan.GetDescription() != "" {
m.Description = types.StringValue(vlan.GetDescription())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty or not, when would we not want the model description to match the upstream description?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be other ways to address the bug fixed by that previous PR; I forget what else, if anything, I tried at that time, but I think I was aiming to maintain backwards compatibility. The effect of this if block is that, if the description is null or "" in the response, it is null in the model. I don't recall if the API meaningfully differentiates between a null VLAN description and an empty VLAN description.


if vlan.Description != "" {
m.Description = types.StringValue(vlan.Description)
if vlan.GetDescription() != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above, why wouldn't we set description unconditionally?

ctreatma added a commit that referenced this pull request Sep 30, 2024
This was broken out of #782 since it involves a widely-used helper
function and will impact more than just the Metal VLAN resource.

Previously, the `IgnoreHttpResponseErrors` helper function accepted as
arguments a number of other, existing helper functions that are meant to
evaluate whether a particular API response has a certain HTTP status.
Here's [an example helper function for checking if the response is a
403](https://github.com/equinix/terraform-provider-equinix/blob/b1ed71b389705513308c1e47e7ac6ff0c46891cd/internal/errors/errors.go#L119-L130):

```go
func HttpForbidden(resp *http.Response, err error) bool {
	if resp != nil && (resp.StatusCode != http.StatusForbidden) {
		return false
	}

	switch err := err.(type) {
	case *ErrorResponse, *packngo.ErrorResponse:
		return IsForbidden(err)
	}

	return false
}
```

This function immediately returns false if the response status code _is
not_ 403; otherwise it tries to convert the error argument to an
`ErrorResponse` or a `packngo.ErrorResponse` and then use a different
helper function to determine if that error represents a 403. This
assumes that the error object contains a copy of the response, which is
only certain to be the case for code that uses `packngo`. In practice we
almost never did the necessary conversion to `ErrorResponse` for
non-`packngo` resources and data sources, so in many cases we were
probably not successfully ignoring the desired response codes.

This refactors `IgnoreHttpResponseErrors` to inspect HTTP status codes
directly in the response instead of relying on helper functions for
specific custom error types.

This also removes the `FriendlyErrorForMetalGo` function which only
existed in order to convert a separate HTTP response and error into an
`ErrorResponse`, which is no longer necessary. Some usage of
`FriendlyError` is also removed in cases where that function had no or
minimal effect.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 suggestion.

Comments skipped due to low confidence (1)

internal/resources/metal/vlan/datasource.go:110

  • The type assertion for 'facility_code' should be checked to avoid potential runtime panic. Use a safe type assertion.
facility_code, ok := v.AdditionalProperties["facility_code"].(string)

client := acceptance.TestAccProvider.Meta().(*config.Config).NewMetalClientForTesting()

foundVlan, _, err := client.ProjectVirtualNetworks.Get(rs.Primary.ID, nil)
foundVlan, _, err := client.VLANsApi.GetVirtualNetwork(context.Background(), rs.Primary.ID).Execute()
Copy link

Copilot AI Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using context.Background() directly in tests is not recommended. Consider using a context that can be controlled or canceled.

Suggested change
foundVlan, _, err := client.VLANsApi.GetVirtualNetwork(context.Background(), rs.Primary.ID).Execute()
foundVlan, _, err := client.VLANsApi.GetVirtualNetwork(context.WithTimeout(context.Background(), time.Second*10), rs.Primary.ID).Execute()

Copilot uses AI. Check for mistakes.
@ctreatma ctreatma closed this Nov 12, 2024
@ctreatma ctreatma deleted the metal-vlan-sdk branch November 12, 2024 19:33
kpdhulipala pushed a commit to kpdhulipala/terraform-provider-equinix that referenced this pull request Mar 24, 2025
This was broken out of equinix#782 since it involves a widely-used helper
function and will impact more than just the Metal VLAN resource.

Previously, the `IgnoreHttpResponseErrors` helper function accepted as
arguments a number of other, existing helper functions that are meant to
evaluate whether a particular API response has a certain HTTP status.
Here's [an example helper function for checking if the response is a
403](https://github.com/equinix/terraform-provider-equinix/blob/b1ed71b389705513308c1e47e7ac6ff0c46891cd/internal/errors/errors.go#L119-L130):

```go
func HttpForbidden(resp *http.Response, err error) bool {
	if resp != nil && (resp.StatusCode != http.StatusForbidden) {
		return false
	}

	switch err := err.(type) {
	case *ErrorResponse, *packngo.ErrorResponse:
		return IsForbidden(err)
	}

	return false
}
```

This function immediately returns false if the response status code _is
not_ 403; otherwise it tries to convert the error argument to an
`ErrorResponse` or a `packngo.ErrorResponse` and then use a different
helper function to determine if that error represents a 403. This
assumes that the error object contains a copy of the response, which is
only certain to be the case for code that uses `packngo`. In practice we
almost never did the necessary conversion to `ErrorResponse` for
non-`packngo` resources and data sources, so in many cases we were
probably not successfully ignoring the desired response codes.

This refactors `IgnoreHttpResponseErrors` to inspect HTTP status codes
directly in the response instead of relying on helper functions for
specific custom error types.

This also removes the `FriendlyErrorForMetalGo` function which only
existed in order to convert a separate HTTP response and error into an
`ErrorResponse`, which is no longer necessary. Some usage of
`FriendlyError` is also removed in cases where that function had no or
minimal effect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants